Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove AvaloniaProperty.Initialized and PseudoClass static methods #3292

Merged
merged 9 commits into from
Feb 12, 2020
Merged

Remove AvaloniaProperty.Initialized and PseudoClass static methods #3292

merged 9 commits into from
Feb 12, 2020

Conversation

grokys
Copy link
Member

@grokys grokys commented Nov 26, 2019 •
edited
Loading

What does the pull request do?

I'd like to get some thoughts on this, it's removing functionality in the pursuit of performance.

Every time an AvaloniaObject is constructed we fire an AvaloniaProperty.Initialized observable for each property registered on the object. This observable is used to automatically apply pseudoclasses to controls using the static PseudoClass() method calls. We've tried to optimize this process, but the performance hit is still quite large.

In this PR, I removed the AvaloniaProperty.Initialized observable and related machinery. I then tried to implement the PseudoClass functionality in what I hoped would be a more performant way - lets call this attempt PseudoClassEngine, you can see the code here. This did indeed improve performance a little, but there was still an overhead.

I then tried completely removing the static PseudoClass helper methods and managing pseudoclasses in code. Performance was much better. Here are some benchmarks:

master

|           Method |     Mean |     Error |    StdDev |  Gen 0 | Gen 1 | Gen 2 | Allocated |
|----------------- |---------:|----------:|----------:|-------:|------:|------:|----------:|
| InitializeButton | 2.545 us | 0.0135 us | 0.0120 us | 1.4534 |     - |     - |   1.37 KB |

PseudoclassEngine

|           Method |     Mean |    Error |   StdDev |  Gen 0 | Gen 1 | Gen 2 | Allocated |
|----------------- |---------:|---------:|---------:|-------:|------:|------:|----------:|
| InitializeButton | 915.4 ns | 1.783 ns | 1.581 ns | 1.1139 |     - |     - |   1.05 KB |

Pseudoclasses in code

|           Method |     Mean |     Error |    StdDev |  Gen 0 | Gen 1 | Gen 2 | Allocated |
|----------------- |---------:|----------:|----------:|-------:|------:|------:|----------:|
| InitializeButton | 246.7 ns | 0.8485 ns | 0.7937 ns | 0.9899 |     - |     - |     952 B |

As you can see, creating an instance of a Button is now 10x faster.

The question is: are we willing to remove functionality like this to get better performance?

Breaking changes

  • AvaloniaProperty.Initialized removed
  • Static PseudoClass methods removed

Note

Targets #3287

@grokys
Remove OperationsPerInvoke here.
1d4a792 
It was incorrect: invoking the method doesn't create 1000 buttons.
@grokys
Remove AvaloniaProperty.Initialized.
35bd69f 
It was only needed for pseudoclasses. Move pseudoclass registration to an engine that doesn't require `AvaloniaProperty.Initialized`, implemented in `PseudoclassEngine`.
@grokys
Set pseudoclasses manually.
b89305d 
It makes creating new controls a _lot_ faster.
@grokys grokys changed the base branch from master to refactor/value-store November 26, 2019 10:36
@grokys grokys changed the base branch from refactor/value-store to refactor/wpf-validation-coercion November 26, 2019 10:37
@JaggerJo
Copy link
Contributor

Never needed AvaloniaProperty.Initialzed and 10x per control creation is a lot.

@grokys grokys changed the title Refactor/remove property initialized Remove AvaloniaProperty.Initialized and PseudoClass static methods Nov 26, 2019
@MarchingCube MarchingCube mentioned this pull request Nov 26, 2019
Merged
3 tasks
@MarchingCube
Copy link
Collaborator

I think that the benefit of dropping Initialized is huge since it also affects startup time of the app, when starting the app initialized property cache is invalidated and created again very often. Plus general simplification of our core codebase should make future optimizations easier to pull off.

@MarchingCube
Copy link
Collaborator

@grokys So what is the verdict here? Should we merge this in?

@grokys
Copy link
Member Author

grokys commented Dec 9, 2019

This targets #3287 and by extension #3255, so it's the last in a chain of quite large changes. I think we should decide when/whether to merge the others first. If we want this but not the others I can port this to target master.

@MarchingCube
Copy link
Collaborator

@grokys Since other PRs got merged are we planning to resume working on this?

@grokys
Copy link
Member Author

grokys commented Jan 31, 2020

Yeah I think this is worth doing, despite the breakage it will cause.

@grokys grokys marked this pull request as ready for review January 31, 2020 14:09
@grokys grokys changed the base branch from refactor/wpf-validation-coercion to master January 31, 2020 14:12
@grokys
Copy link
Member Author

grokys commented Jan 31, 2020

Ok, updated this to target master. Now ready for review.

MarchingCube
MarchingCube suggested changes Feb 1, 2020
View reviewed changes
Copy link
Collaborator

@MarchingCube MarchingCube left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally LGTM, but one issue that I've found must be fixed.

src/Avalonia.Controls/Primitives/ToggleButton.cs Outdated Show resolved Hide resolved
MarchingCube
MarchingCube approved these changes Feb 3, 2020
View reviewed changes
Copy link
Collaborator

@MarchingCube MarchingCube left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@grokys grokys mentioned this pull request Feb 4, 2020
Closed
@grokys grokys merged commit 15140b8 into master Feb 12, 2020
@grokys grokys deleted the refactor/remove-property-initialized branch February 12, 2020 07:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MarchingCube MarchingCube MarchingCube approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@grokys @JaggerJo @MarchingCube